Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚠️ [CLAP] Fix dtype of logit scales in init #25682

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

sanchit-gandhi
Copy link
Contributor

What does this PR do?

The dtype of the CLAP logit scale parameters was always float64 by default (even if the rest of the model was initialised in float32). This PR fixes the logit scales, such that they respect the default dtype of the model.

@sanchit-gandhi sanchit-gandhi marked this pull request as ready for review August 23, 2023 11:43
@@ -1956,8 +1955,8 @@ def __init__(self, config: ClapConfig):
text_config = config.text_config
audio_config = config.audio_config

self.logit_scale_a = nn.Parameter(torch.tensor(np.log(config.logit_scale_init_value)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aforementioned behaviour is a result of the np.log operation defaulting to float64

Copy link
Collaborator

@ArthurZucker ArthurZucker Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the original code we might need to init in float64 then cast to float if it makes a difference. No idea if the actual value save is in float64!

Copy link
Contributor Author

@sanchit-gandhi sanchit-gandhi Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameters are initialised in float64 but are stored in float32 in the state dict

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned offline, never used in the original repo. Is a bit breaking but it is a bug fix. Let's just add one ⚠️ !

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 23, 2023

The documentation is not available anymore as the PR was closed or merged.

@sanchit-gandhi
Copy link
Contributor Author

sanchit-gandhi commented Aug 23, 2023

Note that in the original repo, the model is always cast to float16 for all training / inference. Thus, they likely never used the model in it's default dtype, and always relied on explicitly casting to float16

@sanchit-gandhi sanchit-gandhi changed the title [CLAP] Fix dtype of logit scales ⚠️ [CLAP] Fix dtype of logit scales in init Aug 23, 2023
@sanchit-gandhi sanchit-gandhi merged commit 77cb2ab into huggingface:main Aug 23, 2023
@sanchit-gandhi sanchit-gandhi deleted the clap-dtype branch August 23, 2023 17:49
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants